Data faceting #538
Data faceting #538
Conversation
|
This pull request introduces 1 alert when merging 1949056 into 553c019 - view on LGTM.com new alerts:
|
| import {State} from '../store'; | ||
| import {Dispatch} from 'redux'; | ||
|
|
||
| export function addFacetLayout (payload: FacetLayoutRecord) { |
There was a problem hiding this comment.
re: this file, facetLayoutReducer, and facetLayout.ts, i think they're related enough to core layout functionality that i would consider just adding the facet actions / reducer cases / record definitions to the existing layout files so that it's easier to find everything.
| /** | ||
| * Layouts align multiple groups | ||
| */ | ||
| export interface FacetLayout { |
There was a problem hiding this comment.
is there enough overlap between FacetLayout and Layout that we can think about e.g.:
- a common interface that both of them "inherit" from via the typescript
&(intersection type) operator - a "parent type" defined as the
|(union type) of the two?
i'm not suggesting either particular choice is more right here but consider whether or not that might simplify things in some places (it also might not simplify things at all, in which case feel free to reject this idea)
There was a problem hiding this comment.
Interesting, they are similar in concept but very different in implementation and how they are used in Lyra and the ultimate vega spec. Perhaps worth discussing further
src/js/ctrl/export.ts
Outdated
| const layout = clean(duplicate(facetLayouts), internal); | ||
| const id = Object.keys(layout)[Object.keys(layout).length -1]; | ||
| return layout[id]; | ||
| } |
There was a problem hiding this comment.
if i'm understanding this correctly, the logic is:
- if there are any facet layouts in the lyra store, set the last one created as the layout in the exported spec
a few questions to help my understanding:
- why do we discard the previous ones?
- what are the cases where there will be more than one facet layout?
- how is this different from how non-facet layouts are handled?
There was a problem hiding this comment.
Really wasn't sure on how the logic of the export worked so this might be something we can walk through tomorrow
|
awesome work! i left some comments, questions, and suggestions. exiting to see this getting so close! |
| @@ -52,6 +54,7 @@ const getDefaultState = Record<LyraState>({ | |||
| widgets: Map<string, WidgetRecord>(), | |||
| signals: defaultSignalState, | |||
| layouts: Map<string, LayoutRecord>(), | |||
| facetLayouts: Map<string, FacetLayoutRecord>(), | |||
There was a problem hiding this comment.
suggestion per our conversation today: since there can only be one vega layout spec, change this from a Map to a single FacetLayoutRecord
There was a problem hiding this comment.
Probably just not understanding typescript syntax/types here, but when I change the line to facetLayouts: FacetLayoutRecord I get an error:'FacetLayoutRecord' only refers to a type, but is being used as a value here.
There was a problem hiding this comment.
Here you're initializing the actual values of a record, not defining an interface. So what you're trying to do in this line is set the value of a property facetLayouts to a default FacetLayoutRecord. So it should be facetLayouts: FacetLayoutRecord() -- note the parentheses for initialization
There was a problem hiding this comment.
hmm I'm getting the same error even with the parentheses
There was a problem hiding this comment.
i meant FacetLayout(), for the constructor and not the type
There was a problem hiding this comment.
ohhh make sense. realizing with the proposed change to facet layouts to make it compatible with the other layout mechanism, we could in theory support multiple facets and might want to keep it as a map after all?
No description provided.